Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mellanox] Add bitmap support for SFP error event #7605

Merged
merged 8 commits into from
Jun 25, 2021

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented May 13, 2021

Depends on sonic-net/sonic-platform-daemons#184

Why I did it

Currently, SONiC use a single value to represent SFP error, however, multiple SFP errors could exist at the same time. This PR is aimed to support it

How I did it

Return bitmap instead of single value when a SFP event occurs

How to verify it

  1. New unit test case is added to cover the change
  2. Manual test

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@Junchao-Mellanox Junchao-Mellanox requested a review from lguohan as a code owner May 13, 2021 07:00
@jleveque
Copy link
Contributor

This is pending on the discussion at stephenxs/SONiC#12, with regard to making this a generic feature, if possible.

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox now that it is not single value how the user is expected to understand the errors? should we add some information in the CLI? we cannot assume users has the code :-)

Please confirm which HLD we should refer in this PR. Is it sonic-net/SONiC#586 ?

@Junchao-Mellanox Junchao-Mellanox marked this pull request as draft May 27, 2021 02:07
Junchao-Mellanox and others added 4 commits June 7, 2021 18:32
1. Update get_change_event
   - Return event in a bitmap format in case there is an error
   - Return an extra map indexed by sfp_error in case there is a Mellanox-specific error
   - Indicate whether the error is a blocking error
2. Expose the error status of SFP modules to CLI
   - A platform API calls SDK API to fetch the error status
     - Display the SFP module state: plugged, unplugged and plugged with error.
     - The SFP error state is displayed only if the SFP module state is plugged with error
   - The CLI will call platform API and provide user-friendly output
3. Redirect stderr to /dev/null in order to eliminate errors in case SFP module is not plugged
4. Beautify the code, making all the state/error/descriptions defined in a uniform place

Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
@liat-grozovik liat-grozovik requested review from jleveque and keboliu and removed request for lguohan June 7, 2021 16:03
Support handling the error status of SFP modules
@lgtm-com
Copy link

lgtm-com bot commented Jun 8, 2021

This pull request introduces 4 alerts when merging cb5135f into 62a4603 - view on LGTM.com

new alerts:

  • 4 for Wrong number of arguments in a call

@Junchao-Mellanox Junchao-Mellanox marked this pull request as ready for review June 10, 2021 01:33
@Junchao-Mellanox
Copy link
Collaborator Author

The LGTM warnings are not real issue. It looks like a bug of LGTM.

@Junchao-Mellanox
Copy link
Collaborator Author

@Junchao-Mellanox now that it is not single value how the user is expected to understand the errors? should we add some information in the CLI? we cannot assume users has the code :-)

Please confirm which HLD we should refer in this PR. Is it Azure/SONiC#586 ?

Stephen has updated the HLD, and user could get the error status from CLI.

@stephenxs stephenxs closed this Jun 15, 2021
@stephenxs stephenxs reopened this Jun 15, 2021
@stephenxs
Copy link
Collaborator

Pending on PR sonic-net/sonic-platform-common#194 to be merged first.

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Copy link
Collaborator

@jleveque can you review the PR when you have time? thanks.

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

sonic-net/sonic-platform-daemons#184 has merged. Please update the submodule, then we can merge this PR.

@jleveque jleveque changed the title Add bitmap support for SFP error event [Mellanox] Add bitmap support for SFP error event Jun 22, 2021
@stephenxs
Copy link
Collaborator

Looks good to me.

Azure/sonic-platform-daemons#184 has merged. Please update the submodule, then we can merge this PR.

Thank you. Will do.

@stephenxs
Copy link
Collaborator

Looks good to me.
Azure/sonic-platform-daemons#184 has merged. Please update the submodule, then we can merge this PR.

Thank you. Will do.

advancing submodule PR: #7961

@jleveque jleveque merged commit 147bf24 into sonic-net:master Jun 25, 2021
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
#### Why I did it

Currently, SONiC use a single value to represent SFP error, however, multiple SFP errors could exist at the same time. This PR is aimed to support it

#### How I did it

Return bitmap instead of single value when a SFP event occurs

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@Junchao-Mellanox Junchao-Mellanox deleted the sfp-bit-map branch October 29, 2021 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants